Added support to indicate node.js architecture on Windows (dcodeIO#8)#9
Added support to indicate node.js architecture on Windows (dcodeIO#8)#9javihernandez wants to merge 5 commits intodcodeIO:masterfrom
Conversation
dcodeIO
left a comment
dcodeIO
left a comment
There was a problem hiding this comment.
Thanks, looking good to me! 👍 Just some minor comments.
index.js
Outdated
| let arch = core.getInput("node-arch") || null; | ||
| runScript("powershell", ".\\install.ps1", version, mirror, arch); |
There was a problem hiding this comment.
Won't this pass the string "null" on the command line? Perhaps just ""?
There was a problem hiding this comment.
So true, I just updated it.
.github/workflows/test.yml
Outdated
| - name: Check | ||
| run: | | ||
| node -v | ||
| node -e 'console.log(process.arch)' |
There was a problem hiding this comment.
Can we make this an equality check so the step fails if the value doesn't match the expectation?
There was a problem hiding this comment.
Done, let me know if you prefer a different approach ;)
index.js
Outdated
| let version = await resolveVersion(core.getInput("node-version"), mirror); | ||
| if (process.platform == "win32") { | ||
| runScript("powershell", ".\\install.ps1", version, mirror); | ||
| let arch = core.getInput("node-arch") || ""; |
There was a problem hiding this comment.
should this throw if provided on a different platform?
There was a problem hiding this comment.
Good question. In this particular case, node-arch is only being processed on Windows systems and I marked the input field as "Windows only" in the README file in an attempt to avoid any confusion between nvm and nvm-windows.
As an option, we can move this out of the platform check and indicate that node-arch has no effect on *nix systems when provided, but IMHO, throwing doesn't look like a good idea to me. But of course, I'm happy to hear your thoughts on this.
There was a problem hiding this comment.
I can see the benefit of silently ignoring it, when the user's intention is "only respect this on windows".
However, if the user's intention is "force this architecture", they might end up running non-windows tests on a different architecture than intended, which could mask a lot of problems.
Since it's impossible to know the user's intention, it seems to me like the safer approach is to throw rather than silently ignore the invalid option.
There was a problem hiding this comment.
Sure, I see your point, however, can you elaborate a bit more on:
if the user's intention is "force this architecture", they might end up running non-windows tests on a different architecture than intended, which could mask a lot of problems.
I'm not sure I follow you. How the users might end up doing such thing if the option is ignored on non-windows platforms?
I just want to understand this better, that's all.
Oh, and throwing on non-windows platforms should be as easy as adding a core.setFailed when node-arch has been provided, @dcodeIO would you be okay with it?
There was a problem hiding this comment.
If a user's intention is to ensure their project works on 32 bit and 64 bit machines, and in fact their non-windows tests are only running on 64 bit machines, then their 32-bit users on non-windows machines aren't being covered - the testing is a false positive.
|
Ok, I checked that the action is properly throwing but it wasn't really rejecting to keep going (see a few different CI runs here) |
3 participants
No description provided.